London | 26-SDC-July | Alex Jamshidi | Sprint 4 | Implement shell tools in python - #676
Alex-Jamshidi wants to merge 30 commits into
Conversation
…d through functions as arguments - also fixed an if boolean
Alex-Jamshidi
added
📅 Sprint 4
LonMcGregor
added
the
Review in progress
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Good start on this task. Reading through it, I can see you have put thought into these implementations, but they feel a little bit over-engineered, and at times are a bit difficult to see how the data is flowing through them. I've got a few questions for you to answer which might help me better understand your thinking.
| if not (Path(cwd) / p).is_dir() | ||
| ] | ||
| if not flag_status["show_all"]: | ||
| file_args = remove_dot_files(file_args) |
There was a problem hiding this comment.
There are multiple places where you use remove_dot_files in this program. Is there a reason for that?
There was a problem hiding this comment.
The program takes the original filesystem items (which could be files or folders, or both).
It then splits these items into files (get_file_args) and directories (get_dir_args).
The list of file arguments may contain dot files, like .hidden.txt, so these are removed at this stage.
If directory items are found, these need to be opened up, the contents of which are to be displayed on a separate line, this is done using the dir_output function. These directories may also contain dot files, so these need to be removed.
If the second instance of remove_dot_files were removed:
In the case of the sample files given, none of the sub directories actually contain dot files, so the output of ls and ls.py would be equivalent.
but if I ran, for example: python3 ls.py ./* whereby sample-files is now a subdirectory to be opened.
then when the dir_output function populates the files in sample-files, the .hidden.txt would be incorrectly shown.
There was a problem hiding this comment.
OK, I understand. The dotfiles are treated a little differently in the case of directories that need to be inspected. That makes sense in this case.
LonMcGregor
added
Reviewed
Alex-Jamshidi
added
Needs Review
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
This looks more complete now, good work
| if not (Path(cwd) / p).is_dir() | ||
| ] | ||
| if not flag_status["show_all"]: | ||
| file_args = remove_dot_files(file_args) |
There was a problem hiding this comment.
OK, I understand. The dotfiles are treated a little differently in the case of directories that need to be inspected. That makes sense in this case.
LonMcGregor
added
Complete
2 participants
Learners, PR Template
Self checklist
Task code
CYF-1152
Changelist
Implemented wc, ls, cat in python